fix: widen scalar pixel_scales / shape_native at the two sites #464 missed - #485
Merged
Merged
Conversation
…issed PyAutoArray#464 (`8298d74e`) replaced `type(x) is float` with `validate.is_concrete_scalar` in `convert_pixel_scales_1d` and `convert_pixel_scales_2d`, so any concrete real scalar widens to the tuple form both functions promise. Re-running that prompt's repro found the sweep did not reach every site of the same defect. Two were still live on main. `Mask1D.__init__` hand-rolled its own widening and never routed through `convert_pixel_scales_1d`, so it still carried the original exact-type check. `Mask1D(mask=..., pixel_scales=1)` stored the bare `1`, and the mask's geometry then raised `TypeError: 'int' object is not subscriptable` — #464's exact reported symptom, on a public constructor. `Mask2D.__init__` already called `convert_pixel_scales_2d`, and `Grid1D.uniform` reaches the chokepoint too, so this was a 1D/2D divergence rather than a design choice. It now makes the same call `Mask2D` makes. That also brings `validate.validate_pixel_scales` to `Mask1D`, which is a deliberate contract change: `Mask1D` now rejects `0`, negative and `nan` pixel scales exactly as `Mask2D` already did. No test constructed one that way and all 12 library call sites pass real scales, so nothing needed adjusting to suit it. `convert_shape_native_1d` kept `type(shape_native) is int`, which `8298d74e` listed as not-fixed-there. `Array1D.full` is its sole caller and does `shape_native[0]` on the result, so `Array1D.full(shape_native=np.int32(5))` raised `IndexError: invalid index to scalar variable`. It now tests `validate.is_concrete_integer` and casts to a Python `int`. `is_concrete_integer` is new, beside `is_concrete_scalar`: `shape_native` counts pixels rather than measuring them, so `is_concrete_scalar` is the wrong predicate there — it would silently widen a `float`, which is a mistake worth surfacing. `bool` exclusion and tracer-safety carry over unchanged, so both functions stay safe inside a `jax.jit`; verified by compiling and running one. Also tightened #464's own widening tests. `np.float64(1.0) == (1.0,)` NumPy-broadcasts to `array([True])`, which is truthy, so their value-only assertions passed on an unwidened NumPy scalar and tested nothing. Asserting tuple-ness before the value makes them fail on the pre-#464 source (confirmed by reverting it), where four of the six parametrisations previously passed vacuously. The new tests here assert the same way for the same reason. Not fixed here, needing its own change: tuple entries are still returned unnormalised, so `convert_pixel_scales_2d((1, 1))` keeps its ints and contradicts the `Tuple[float, float]` annotation. That alters return values on paths which work today. Validation: 1201 passed / 0 failed on the full test_autoarray suite. The 3 pre-existing pynufft failures `8298d74e` baselined no longer occur, so there was nothing to baseline against. Every new assertion that claims regression coverage was confirmed to fail without the source change; the boundary tests (tuple unchanged, float/bool not widened, tracer passthrough) pass either way by design, mirroring the ones #464 shipped. Downstream blast radius is nil: PyAutoGalaxy and PyAutoLens only re-export `Mask1D` and construct none, and neither uses `Array1D.full`/`zeros`/`ones`. Closes #484 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fj1HoQa4hZPmbyyBYNJX62
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PyAutoArray#464 (
8298d74e) replacedtype(x) is floatwithvalidate.is_concrete_scalarin
convert_pixel_scales_1d/convert_pixel_scales_2d. Re-running that prompt's reprofound the sweep did not reach every site of the same defect. Two were still live on
main,both reproduced before being touched:
Mask1D.__init__hand-rolled its own widening and never routed throughconvert_pixel_scales_1d, so it still carried the original exact-type check.Mask1D(mask=…, pixel_scales=1)stored the bare1, and geometry then raisedTypeError: 'int' object is not subscriptable— fix: widen int / numpy scalar pixel_scales to a tuple #464's exact reported symptom, on apublic constructor.
Mask2D.__init__already calledconvert_pixel_scales_2d, so thiswas a 1D/2D divergence rather than a design choice.
convert_shape_native_1dkepttype(shape_native) is int, which8298d74elistedas not-fixed-there.
Array1D.fullis its sole caller and doesshape_native[0]on theresult, so
Array1D.full(shape_native=np.int32(5))raisedIndexError: invalid index to scalar variable.Closes #484.
API Changes
Mask1Dnow widens any concrete real scalarpixel_scales(int,np.integer,np.floating) to(float,), where previously only an exactfloatwas widened andanything else was stored bare. Routing it through the shared chokepoint also brings
validate.validate_pixel_scaleswith it, soMask1Dnow rejects0, negative andnanpixel scales — a deliberate contract change that makes it matchMask2D, whichalready rejected them.
convert_shape_native_1dlikewise widens any concrete integerscalar and casts to a Python
int. One new public predicate,validate.is_concrete_integer.No symbol was removed or renamed, and no signature changed.
See full details below.
Test Plan
test_autoarraysuite: 1201 passed, 0 failed. The 3 pre-existing pynufftfailures
8298d74ebaselined no longer occur, so there was nothing to baseline against.source change. The boundary tests (tuple returned unchanged,
float/boolnotwidened, tracer passthrough) pass either way by design, mirroring those fix: widen int / numpy scalar pixel_scales to a tuple #464 shipped.
jax.jitcompiles and runs with a tracedpixel_scales, which passesthrough untouched — both functions stay
jit-safe.Mask1Dandconstruct none; neither uses
Array1D.full/zeros/ones.Test-quality fix included
#464's own widening tests asserted value only, and
np.float64(1.0) == (1.0,)NumPy-broadcasts to
array([True]), which is truthy — so they passed on an unwidened NumPyscalar and tested nothing. Four of six parametrisations were vacuous. They now assert
tuple-ness before the value, confirmed by reverting to the pre-#464 source and watching them
fail. The new tests here assert the same way for the same reason.
Not fixed here
Tuple entries are still returned unnormalised:
convert_pixel_scales_2d((1, 1))keeps itsints and contradicts the
Tuple[float, float]annotation. It alters return values on pathsthat work today, so it needs its own change and its own suite read.
Full API Changes (for automation & release notes)
Added
autoarray.validate.is_concrete_integer(value)—Truefor a concrete Python or NumPyinteger scalar. The integer-only counterpart of
is_concrete_scalar, for parameterswhich count pixels rather than measure them: a
floatreturnsFalsehere whereis_concrete_scalaraccepts it.boolexclusion and tracer-safety carry over unchanged.Changed Behaviour
autoarray.Mask1D.__init__—pixel_scalesgiven as any concrete real scalar (int,np.integer,np.floating) is now widened to(float,). Previously only an exactfloatwas widened; anything else was stored bare and raisedTypeError: 'int' object is not subscriptableon first geometry use.autoarray.Mask1D.__init__— now raisesValueErrorforpixel_scalesof0, anegative number, or
nan, in both scalar and tuple form. Previously accepted silently.This matches
Mask2D, which already validated.autoarray.util.geometry.convert_shape_native_1d—shape_nativegiven as any concreteinteger scalar (
int,np.integer) is now widened to(int,)and cast to a Pythonint. Previously only an exactintwas widened; annp.integerfell through andArray1D.full/zeros/onesraisedIndexError: invalid index to scalar variable.A
floatis still deliberately not widened.Removed
None.
Renamed
None.
Changed Signature
None.
Migration
No migration required — all three changes accept strictly more input than before, except
the
Mask1Dvalidation, which rejects pixel scales that were already unusable:Mask1D(mask=…, pixel_scales=0.0)constructed, then produced a division by zeroin every pixel-to-scaled conversion.
Mask1D(mask=…, pixel_scales=0.0)raisesValueErrornaming the parameter andthe received value.
Generated by the PyAutoLabs agent workflow.